Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

cmd/swarm/swarm-snapshot: Swarm snapshot binary #1077

Closed
wants to merge 11 commits into from
Closed

Conversation

acud
Copy link
Member

@acud acud commented Dec 20, 2018

No description provided.

Copy link
Contributor

@nolash nolash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an exhaustive review. For example, I have not reviewed the tests, since I have several questions about the implementation, which may cause the premise of the tests to change.

p2p/simulations/network.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia.go Outdated Show resolved Hide resolved
p2p/simulations/simulation.go Outdated Show resolved Hide resolved
@acud
Copy link
Member Author

acud commented Dec 27, 2018

@nolash the puritan approach of trying to scope the usage of components to be mainly from the p2p package is biting me in the axx. the swarm simulations package stitches additional abstractions over the p2p ones. notably:
https://github.com/ethersphere/go-ethereum/blob/9e9fc87e70accf2b81be8772ab2ab0c914e95666/swarm/network/simulation/simulation.go#L66

https://github.com/ethersphere/go-ethereum/blob/9e9fc87e70accf2b81be8772ab2ab0c914e95666/p2p/simulations/adapters/types.go#L231

right now i'm using the swarm simulations package just to wait for the network health (basically just creating an empty simulation and injecting the network object into it, then calling the health function).

I think that this really isn't necessary to have this method with a simulation as a receiver, but rather a function that just takes a Network as an argument.

janos
janos previously requested changes Jan 2, 2019
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/main.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/verify_test.go Outdated Show resolved Hide resolved
internal/cmdtest/test_cmd.go Outdated Show resolved Hide resolved
p2p/simulations/network.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/verify.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create_test.go Show resolved Hide resolved
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general i am very doubtful about this entire PR.
it is a lot of code for something that already existed plus stuff that is not needed.
lets discuss

cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
cmd/swarm/swarm-snapshot/create.go Outdated Show resolved Hide resolved
swarm/network/simulation/kademlia.go Outdated Show resolved Hide resolved
@acud
Copy link
Member Author

acud commented Jan 3, 2019

@zelig,
I can understand your concerns with this PR.
My view on this is that maybe this is not really obviously necessary, but I personally don't like the fact that snapshot generation is being handled inside an e2e test.
Therefore, I think that splitting this to a separate binary is indeed the way to go. Also, I have to say that working through the swarm simulations and p2p simulations helps a lot to see where we need to clean up and where are our current weaknesses in the way we use the simulations framework. Speaking of duplicate code, there's a lot of it there and there's still room to make it cleaner and better so we can really finish the work on this piece of the puzzle. Speaking for myself, I think we still can't fully reason about the simulations FW, and hopefully cleaning stuff up will help resolve the flakiness that we have every now and then.

That being said, there's certain boilerplate code that is indeed replicated and unnecessary, and maybe we should think about having just one toolbox binary that includes, for example, smoke tests, snapshot generation, snapshot verification and any other need that might arise further down the road for that matter.

@zelig
Copy link
Member

zelig commented Jan 10, 2019

@acud
Copy link
Member Author

acud commented Jan 15, 2019

@nolash @zelig @janos i removed the verification command from the binary and the snapshot json. verification will be relied upon testing.

@acud acud closed this Jan 16, 2019
@acud acud deleted the swarm-snapshot branch January 16, 2019 14:01
@acud acud restored the swarm-snapshot branch January 16, 2019 14:01
@acud acud deleted the swarm-snapshot branch January 16, 2019 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants